Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

running lightly magic, the output is weird #413

Merged
merged 3 commits into from
Jun 17, 2021
Merged

running lightly magic, the output is weird #413

merged 3 commits into from
Jun 17, 2021

Conversation

MalteEbner
Copy link
Contributor

@MalteEbner MalteEbner commented Jun 17, 2021 •
edited
Loading

closes https://github.com/lightly-ai/lightly-core/issues/699

Description

Adds the option to change the trainer.weights_summary. This option is used by pytorch lightning: https://pytorch-lightning.readthedocs.io/en/stable/common/trainer.html#weights-summary

Double print

The double printing is caused by a pytorch lightning bug: Lightning-AI/pytorch-lightning#3458

  • on pytorch-lightning==1.04: working, but double print
  • on pytorch-lightning==1.3.5: working, without double print

image

Tests

  • My change needs new tests
  • I have added/adapted tests accordingly.
  • I have manually tested the change.

If applicable, describe the manual test procedure, e.g:

pip uninstall lightly
export BRANCH_NAME="699-running-lightly-magic,-the-output-is-weird-me"
pip install "git+https://github.com/lightly-ai/lightly.git@$BRANCH_NAME"
lightly-train input_dir='/datasets' trainer.max_epochs=0 trainer.weights_summary=None

Documentation

  • I have added docstrings to all changed/added public functions/methods.
  • My change requires a change to the documentation ( .rst files).
  • I have updated the documentation accordingly.
  • The autodocs update the documentation accordingly.

@codecov
Copy link

codecov bot commented Jun 17, 2021 •
edited
Loading

Codecov Report

Merging #413 (c8e0a86) into master (63b12f0) will increase coverage by 0.09%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #413      +/-   ##
==========================================
+ Coverage   85.15%   85.25%   +0.09%     
==========================================
  Files          74       74              
  Lines        2547     2550       +3     
==========================================
+ Hits         2169     2174       +5     
+ Misses        378      376       -2
Impacted Files Coverage Δ
lightly/embedding/_base.py 83.67% <ø> (ø)
lightly/cli/train_cli.py 89.15% <100.00%> (+2.90%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 63b12f0...c8e0a86. Read the comment docs.

@MalteEbner MalteEbner changed the title 699 running lightly magic, the output is weird me running lightly magic, the output is weird Jun 17, 2021
philippmwirth
philippmwirth reviewed Jun 17, 2021
View reviewed changes
Copy link
Contributor

@philippmwirth philippmwirth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Can you please

  • create the follow-up issue to update the documentation
  • check for compatability with pytorch-lightning 1.0.4 (oldest version we support)

@MalteEbner
Copy link
Contributor Author

  • create the follow-up issue to update the documentation

Where does it need to be updated? The following should be updated automatically by the autodocs: https://docs.lightly.ai/lightly.cli.html#module-lightly.cli.train_cli

@philippmwirth
Copy link
Contributor

philippmwirth commented Jun 17, 2021 •
edited
Loading

  • create the follow-up issue to update the documentation

Where does it need to be updated? The following should be updated automatically by the autodocs: https://docs.lightly.ai/lightly.cli.html#module-lightly.cli.train_cli

I'm just referring to this:
grafik

To me it looked like there was the need for an update but it's not done yet. Hence my request.

Ah but I didn't see that the autodocs take care of it.

@MalteEbner
Copy link
Contributor Author

  • check for compatability with pytorch-lightning 1.0.4 (oldest version we support)

See updated PR description, it is compatible.

@MalteEbner MalteEbner merged commit 8972b58 into master Jun 17, 2021
@MalteEbner MalteEbner deleted the 699-running-lightly-magic,-the-output-is-weird-me branch June 17, 2021 11:45
@MalteEbner MalteEbner restored the 699-running-lightly-magic,-the-output-is-weird-me branch June 18, 2021 09:04
@MalteEbner MalteEbner deleted the 699-running-lightly-magic,-the-output-is-weird-me branch June 18, 2021 09:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@philippmwirth philippmwirth philippmwirth approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@MalteEbner @philippmwirth